WPB-28422 reconcile stale local memberships for deleted remote conversations - #5504
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces reconciliation logic to clean up stale local membership records when a remote (federated) conversation is definitively absent from the owning backend’s successful response, and adds integration coverage for this behavior.
Changes:
- Reconcile (delete) stale local memberships when a remote “get conversations” call succeeds but omits locally-tracked remote conversation IDs.
- Add integration tests that validate reconciliation behavior (including idempotency) and ensure memberships are preserved on federation failures.
- Update an existing integration test to tolerate “no-conversation” results for a one-to-one conversation after remote user deletion.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| libs/wire-subsystems/src/Wire/ConversationSubsystem/Query.hs | Adds reconciliation side-effect when remote conversations are missing from a successful federated response. |
| integration/test/Test/Conversation.hs | Adds integration tests for stale membership reconciliation and adjusts expectations for one-to-one conversation retrieval after remote user deletion. |
| changelog.d/6-federation/WPB-28422 | Documents the new stale-membership cleanup behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
256eb56 to
0caac72
Compare
| handleFailure (Right c) = pure . Right . traverse (.convs) $ c | ||
| handleFailure locallyFound (Right response) = do | ||
| let locallyFoundForDomain = filter ((== tDomain response) . tDomain) locallyFound | ||
| returnedIds = Set.fromList $ map (qualifyAs response . (.id)) (tUnqualified response).convs |
There was a problem hiding this comment.
Looks too complicated to unqualified and then qualify, doesn't Remote a have a functor?
There was a problem hiding this comment.
yeah, the problem is, we need a Set (Remote ConvId) and not Remote (Set ConvId), and I think there is no built in generic operation that does this.
| unless (null remoteNotFoundRemoteIds) $ | ||
| -- FUTUREWORK: This implies that the backends are out of sync. Maybe the | ||
| -- current user should be considered removed from this conversation at this | ||
| -- point. | ||
| P.warn $ | ||
| Logger.msg ("Some locally found conversation ids were not returned by remotes" :: ByteString) | ||
| . Logger.field "convIds" (show remoteNotFoundRemoteIds) |
There was a problem hiding this comment.
Do we need any of this again here?
There was a problem hiding this comment.
This is calling the code above.
blackheaven
left a comment
There was a problem hiding this comment.
We need an extra non-regression tests:
-- | Fetching stale remote conversations from two different domains in one
-- request reconciles both independently and only from successful responses.
testReconcileStaleMembershipsMultipleDomains :: (HasCallStack) => App ()
testReconcileStaleMembershipsMultipleDomains = do
resourcePool <- asks resourcePool
runCodensity (acquireResources 1 resourcePool) $ \[remoteBackend] ->
runCodensity (startDynamicBackend remoteBackend mempty) $ \_ -> do
alice <- randomUser OwnDomain def
ownerStatic <- randomUser OtherDomain def
ownerDynamic <- randomUser remoteBackend.berDomain def
connectTwoUsers ownerStatic alice
connectTwoUsers ownerDynamic alice
convStatic <- registerMissingRemoteConversation ownerStatic [alice]
convDynamic <- registerMissingRemoteConversation ownerDynamic [alice]
eventually $ do
assertConversationMembership alice convStatic True
assertConversationMembership alice convDynamic True
bindResponse (listConversations alice [convStatic, convDynamic]) $ \resp -> do
resp.status `shouldMatchInt` 200
resp.json %. "found" `shouldMatch` ([] :: [Value])
resp.json %. "failed" `shouldMatch` ([] :: [Value])
notFound <- resp.json %. "not_found" & asList
for_ [convStatic, convDynamic] $ \conv -> do
(notFound :: [Value]) `shouldContain` [conv]
assertConversationMembership alice convStatic False
assertConversationMembership alice convDynamic False
-- | Conversations the remote still returns are preserved; only omitted ones
-- are reconciled within the same request.
testReconcileOnlyMissingConversations :: (HasCallStack) => App ()
testReconcileOnlyMissingConversations = do
alice <- randomUser OwnDomain def
owner <- randomUser OtherDomain def
connectTwoUsers owner alice
alive <-
postConversation owner (defProteus {qualifiedUsers = [alice]})
>>= getJSON 201
aliveQid <- objQidObject alive
stale <- registerMissingRemoteConversation owner [alice]
eventually $ do
assertConversationMembership alice aliveQid True
assertConversationMembership alice stale True
bindResponse (listConversations alice [aliveQid, stale]) $ \resp -> do
resp.status `shouldMatchInt` 200
resp.json %. "failed" `shouldMatch` ([] :: [Value])
found <- resp.json %. "found" & asList
length (found :: [Value]) `shouldMatchInt` 1
notFound <- resp.json %. "not_found" & asList
(notFound :: [Value]) `shouldContain` [stale]
assertConversationMembership alice aliveQid True
assertConversationMembership alice stale False
https://wearezeta.atlassian.net/browse/WPB-28422
Checklist
changelog.d